While this may fix #259, it can very easily be a very quiet breaking change for many other use cases. I'd recommend we not merge anything like this until a major release.

Yeah, I couldn't see any way to avoid that. Could add a flag to enable the new behaviour but not sure how you'd feel about that.

Will need to do lots of scenario testing here, but I don't disagree with the change itself - I think consistent behavior is probably the best option given the confusion of the current one. It just has to wait for a major release - I don't think a switch is really worth the complication it brings here. Will wait to see what @mgravell thinks though.

Wouldn't it be safer and clearer (in LookupDbType) just to change the line
handler = null;
to
typeHandlers.TryGetValue(type, out handler);

This would ensure custom TypeHandlers are called for built-in types and enums, whilst minimising side effects.

@peterthomastlc the problem is it changes behaviour for existing apps. If an existing app had a custom type handler for and enum before this change the handler wouldn't have been taking effect. If they rely on that behaviour after the change they would start getting an incorrect result. At best it would crash, worse you'd get data corruption.

This would affect more than just enums as well since it moves custom type handlers to a higher priority than other in-built checks such as type maps and IEnumerable.

As such the only viable option I can see is a flag to set the custom type handler's priority if this change were to be introduced. Though that has the downside of increasing the complexity of the API for users and the complexity when it comes to maintenance and testing (as now you have 2 paths to test depending on this flag).

This is only one issue surrounding type handling that makes me think a better model could be added instead of hacks to support this one specific issue. Then custom and in-built could all use the same pattern, and so that custom could optionally support their own IL generation to optimise get/set as per #433. Though such a change wouldn't be trivial (especially when handling backward compatibility with the old model).

@chilversc So wait, you're suggesting that someone might have:

1. Had fully working mapping code out of the box.
2. Decided to write a custom type mapper, even though the out of the box functionality was working.
3. Added code to register the type mapper.
4. Observed that the type mapper does nothing.
5. Relied on their mapper code being ineffective and dead, so that the built in mapping functionality would be invoked, for the the rest of the app to continue working.

Now, I'm not saying it's impossible. It could happen. But don't you think that this would be both exceedingly rare and extremely silly? I cannot fathom a case in which someone would even create a type mapper in the first place if they rely on the current behavior because relying on the current behavior necessarily implies that Dapper's mapping code is just working for them out of the box, doesn't it?

Are you sure it's worth hampering everyone not using SQL Server (e.g., Oracle above and PostgreSQL and forget the convention of using 'Y' and 'N' varchar in Oracle) for the extremely slim possibility of breaking a ridiculous use case? Do you understand how difficult it is to use Dapper without SQL Server because of this? This is not some minor issue only a few people encounter. This is likely a deal breaker issue that prevents Dapper from even being used in a number of shops that aren't full MS stack.

@bladeoflight16 The more likely scenario would be

1. writing a custom type mapper
2. custom type mapper does not work
3. change code to work with Dapper out of the box
4. left Custom type mapper include, because it wasn't breaking tests

I mean who hasn't tried fixing something 9 different ways and left tries 1-8 in the code because maybe you needed 1 or 2 of those changes as well as the final fix.

I'm still not seeing at what point someone would decide to write a type mapper and register it. Before they introduced Dapper? That doesn't make any sense. After they introduced it and it worked out of the box? Also does not make sense. When you have a problem later on and Dapper is already returning the right data? Also doesn't make sense.

How in the world would this be common enough to justify shooting so many other users in the foot?

Aside: If you don't know what changes you made fixed the bug, you don't know whether it's fixed or not, even if the tests all pass. In all likelihood, it's still broken, but perhaps in new ways. "Looks like it works when I run it" is not the same thing as fixed.

This is where OSS falls down. Major bug in software, 6 years and no fix merged.

Maybe this helps. I combined some of the ideas I found online into something that currently works

https://gist.github.com/djimmo/b54a1643f15bd4e54303992ddce4a968